added a comment + added debug assert on such unwrap_uncheckeds

split up benchmarks to better illustrate the disappointment that started #62924

@bors try @rust-timer queue

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit bb0c01770e325d66292ee8ea84ef1eaf1ba249af with merge 078cf1207b1bbbf628b7e0b76de039fea4fb6d59...

☀️ Try build successful - checks-actions
Build commit: 078cf1207b1bbbf628b7e0b76de039fea4fb6d59 (078cf1207b1bbbf628b7e0b76de039fea4fb6d59)

Queued 078cf1207b1bbbf628b7e0b76de039fea4fb6d59 with parent 35fff69, future comparison URL.

Finished benchmarking try commit (078cf1207b1bbbf628b7e0b76de039fea4fb6d59): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

Lots of red, which to me means, if anything, that #74615 is still haunting unwrap_unchecked. So I could revert back to unwrap for these, or others, but earlier in #74693 you seemed to prefer not to.

As to library/alloc benchmarks:

• Reverting to unwrap in deallocating_next_unchecked influences the times of various benchmarks a bit, including the only benchmarks that actually invoke that function (clone_X_and_into_iter), but they don't stand out at all. Which seems normal because the change is tiny compared to all the allocation and deallocation happening.
• Reverting to unwrap in deallocating_next_back_unchecked doesn't change timing at all (barring the 2% noise of cosmic radiation), which is comforting because reverse iteration is not benchmarked.
• Reverting both, thus just reorganizing functions, still influences timing compared to master. Also (but differently) when introducing inlines.

I am happy with modifications to use of unwrap_unchecked; generally, if we can not use it and the performance does not show problems, we shouldn't use it. (Obviously, if performance improves, then not using it seems better). I think this may be a change in opinion from the other PR; I'd be ok accepting a rebased version of it presuming perf.rlo was neutral or an improvement.

I'd be ok accepting a rebased version of it presuming perf.rlo was neutral or an improvement.

#74693 was about the btree's private unwrap_unchecked, now we use the one Option shares. I tried the equivalent of changing the unwrap_unchecked to unwrap in all btree code, many months ago, and it didn't help performance like it did at the time of #74693. That's just a simple micro-optimisation of the iterator code. Very recently, I tried unifying the separate _checked and _unchecked implementations, that this PR merely makes stand out more. It leaves the code a lot more pleasant to read & write, but performance suffers significantly. But all that performance talk is according to the alloc benchmarks, and I have no idea how it would stand up in court.

Anyway, for the time being, here's a rebased version with the two unwrap reverted, and the new functions that used to be literally inlined #[inline]'d (including two that already existed as private Range methods but are now public in a private module, whatever difference that makes). If this still isn't performance neutral, I'm at a loss.

@bors try @rust-timer queue

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit b9d43c6 with merge 013ba6f7c5a4503fd819fadde66647e054355c7c...

☀️ Try build successful - checks-actions
Build commit: 013ba6f7c5a4503fd819fadde66647e054355c7c (013ba6f7c5a4503fd819fadde66647e054355c7c)

Queued 013ba6f7c5a4503fd819fadde66647e054355c7c with parent eab201d, future comparison URL.

Finished benchmarking try commit (013ba6f7c5a4503fd819fadde66647e054355c7c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@bors r+

📌 Commit b9d43c6 has been approved by Mark-Simulacrum

⌛ Testing commit b9d43c6 with merge 03b845a...

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 03b845a to master...